[feat][storage] Add Span Kind support for ES/OS #6399
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6399 +/- ##
=======================================
Coverage 96.04% 96.05%
=======================================
Files 363 363
Lines 20677 20719 +42
=======================================
+ Hits 19859 19901 +42
Misses 624 624
Partials 194 194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -124,8 +124,9 @@ func getSpanAndServiceIndexFn(p SpanWriterParams) spanAndServiceIndexFn { | |||
| func (s *SpanWriter) WriteSpan(_ context.Context, span *model.Span) error { | |||
| spanIndexName, serviceIndexName := s.spanServiceIndex(span.StartTime) | |||
| jsonSpan := s.spanConverter.FromDomainEmbedProcess(span) | |||
| kind, _ := span.GetSpanKind() | |||
There was a problem hiding this comment.
Why not add Kind field to the dbmodel.Span and not pass it around separately?
There was a problem hiding this comment.
It will be present as a tag in json model also! I tried fetching kind from original span but failed because of the bool AllTagsAsFields. As it will already be present in the json model, should we resave it as a seperate kind also?
Then we can't get old data! In old data |
|
@yurishkuro Comitted as per your suggestions. I tried many ways but I couldn't merge those with |
|
please update the branch, I am not sure if CI is failing because of that or because of your changes. If it's the latter, how are you testing this change? Did you run e2e tests locally? |
I tried approaching with empty kind but failed as query in |
|
@yurishkuro Saving empty strings in ES is not optimal. Please see: elastic/elasticsearch#7515. I have been trying various ways of employing empty or null kinds but not getting results. The probable reason is because query is a bit complex and unconventional. I think it will be better to not to keep empty and null kinds rather handelling them seperately! |
|
I don't have a strong opinion, but the issue you linked talks about searching for empty strings, which I don't think is the case for our scenario, we just need to write it. |
But while reading we have to search for those operations also which have empty kinds or no kind! I am facing problems when fetching spans of all kinds, the search query behaves weirdly if I introduce filter of |
|
Searching for "all kinds" to me means not specifying any filter for the "kind" field. How would it even work if you say |
I think there is some sort of misunderstanding of my approach. Let me first explain problems:
|
|
"Not available in our abstraction" is an odd argument since we own the abstraction and can change it at will. Re fetchsource, what is the source here - is it the whole span? Or do we write separate entries just for service/operation? |
Service, Operation and Kind (if present) but have to investigate the indices whether it is extracting any other information. |
|
Will try to clean this PR as soon as possible |
I have verified! It's only the service model, have comitted with |
|
@yurishkuro I have committed with |
| Query(serviceQuery). | ||
| IgnoreUnavailable(true). | ||
| Aggregation(operationsAggregation, serviceFilter) | ||
|
|
||
| FetchSourceContext(elastic.NewFetchSourceContext(true).Include(spanKind, operationNameField)). |
There was a problem hiding this comment.
I don't really understand this, can you please point to documentation of how this impacts the query behavior?
There was a problem hiding this comment.
https://www.elastic.co/guide/en/elasticsearch/reference/current/search-fields.html#field-retrieval-methods. This part of docs suggests to use fields over source, but I used FetchSourceContext because:
- Weirdly olivere doesn't have
Fieldsin theirSearchService(maybe it's depreceated that's why) but rather it is inExplainService. - Technically we need the whole source as Service mapping has only three fields: Service, Kind, Operation Name.
Manik2708
changed the title
|
@yurishkuro Please can you review this! |
Signed-off-by: Manik2708 <[email protected]>
|
Is this good to go? @yurishkuro |
Signed-off-by: Manik Mehta <[email protected]>
|
is it possible to get code coverage up? |
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
|
@mahadzaryab1 Have completed the code-coverage. A humble reminder to review this PR! |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces Span Kind support for Elasticsearch and OpenSearch, enhancing the querying capabilities of the GetOperations API. The changes include modifications to the data model, reader logic, and test cases to accommodate the new SpanKind field. Overall, the code is well-structured and addresses the issue effectively.
Summary of Findings
Merge Readiness
The code appears to be in good shape and ready for merging. The changes are well-tested and address the intended functionality. I am unable to approve the pull request, and recommend that users have others review and approve this code before merging.
There was a problem hiding this comment.
PR Overview
This PR adds support for span kind filtering when querying operations in Elasticsearch/OpenSearch storage. Key changes include:
- Updating service and writer methods to accept an additional span kind parameter.
- Modifying Elasticsearch query construction in the service layer to include the span kind.
- Updating tests and mocks as well as client and wrapper interfaces to support the new functionality.
Reviewed Changes
| File | Description |
|---|---|
| pkg/es/mocks/SearchService.go | Added a new FetchSourceContext mock method for the updated SearchService interface. |
| internal/storage/v1/elasticsearch/spanstore/service_operation.go | Modified Write and getOperations methods to include and handle span kind; added helper functions for converting aggregation buckets. |
| internal/storage/v1/elasticsearch/spanstore/reader.go | Updated GetOperations to pass the span kind parameter to the storage layer. |
| internal/storage/v1/elasticsearch/spanstore/writer.go | Updated service writer type and its invocations to forward span kind information. |
| pkg/es/client.go & pkg/es/wrapper/wrapper.go | Extended the SearchService interface and its wrapper to include FetchSourceContext. |
| Integration tests (elasticsearch_test.go & opensearch_test.go) | Removed the obsolete flag regarding missing span kind to validate proper behavior. |
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
internal/storage/v1/elasticsearch/spanstore/service_operation.go:59
- Update the function comment to mention the new 'kind' parameter for clarity on its usage.
func (s *ServiceOperationStorage) Write(indexName string, jsonSpan *dbmodel.Span, kind model.SpanKind) {
internal/storage/v1/elasticsearch/spanstore/reader.go:301
- Ensure that 'query.SpanKind' is populated correctly when filtering operations by span kind to avoid unexpected results.
operations, err := s.serviceOperationStorage.getOperations(ctx, jaegerIndices, query.ServiceName, query.SpanKind, s.maxDocCount)
pkg/es/client.go:63
- Verify that all implementations of the SearchService interface now properly handle the new FetchSourceContext method.
FetchSourceContext(fetchSourceContext *elastic.FetchSourceContext) SearchService
pkg/es/wrapper/wrapper.go:268
- Consider adding unit tests for the new FetchSourceContext wrapper method to ensure it correctly chains and returns the expected SearchService.
func (s SearchServiceWrapper) FetchSourceContext(fetchSourceContext *elastic.FetchSourceContext) es.SearchService {
Which problem is this PR solving?
Fixes: #1923
Description of the changes
GetOperations, operations can now be fetched with kind also. When kind kept empty, spans of all kinds are returnedHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test